Skip to content

Support IMAP accounts in delete-staged command#223

Merged
wesm merged 3 commits intowesm:mainfrom
DominicHolmes:fix/imap-delete-staged
Apr 7, 2026
Merged

Support IMAP accounts in delete-staged command#223
wesm merged 3 commits intowesm:mainfrom
DominicHolmes:fix/imap-delete-staged

Conversation

@DominicHolmes
Copy link
Copy Markdown
Contributor

@DominicHolmes DominicHolmes commented Mar 26, 2026 •
edited by wesm
Loading

Human note: this fixed my use case (processing a ton of iCloud-hosted emails). Code looks reasonable to me, but I don't know golang. iCloud (IMAP) path is tested, Gmail path is untested (I don't have a gmail).

===========

Summary

  • delete-staged now detects account type and routes IMAP accounts through the IMAP client path instead of always assuming Gmail OAuth
  • Reuses the existing buildAPIClient factory (same one used by sync) to avoid duplicating IMAP client construction
  • Preserves full Gmail scope escalation behavior (both proactive and reactive paths)

Closes #221

Testing

Tested end-to-end on a real iCloud IMAP account:

  • 10-message batch: 10/10 succeeded, 0 failed (8s). Batch delete correctly fell back to per-message UID STORE \Deleted + UID EXPUNGE.
  • 741-message batch: 741/741 succeeded, 0 failed (~7min). Confirmed the fallback path handles large batches reliably.
  • --dry-run works correctly for IMAP accounts
  • All existing tests pass (make test)
  • go fmt and go vet clean

@DominicHolmes DominicHolmes changed the title Support IMAP accounts in delete-staged command [Opus 4.6] Support IMAP accounts in delete-staged command Mar 26, 2026
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 26, 2026

roborev: Combined Review (ef50564)

Verdict: The PR successfully extends staged deletions to IMAP, but introduces a high-severity risk of targeting the wrong backend for accounts with
multiple sources.

High

  • Location: cmd/msgvault/cmd/deletions.go:367
  • Problem: The new source resolution picks the first
    gmail or imap source returned for an account identifier, but delete-staged still groups manifests by m.Account only. If a user has both a Gmail source and an IMAP source with the same identifier/email, this path can silently target the wrong backend and apply staged deletions against
    the wrong source.
  • Fix: Make staged deletions identify the source unambiguously (for example by persisting source_id/source_type in the manifest and grouping by that), or at minimum fail when GetSourcesByIdentifier returns more than one eligible source instead of taking the first match.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm wesm changed the title [Opus 4.6] Support IMAP accounts in delete-staged command Support IMAP accounts in delete-staged command Apr 7, 2026
@wesm wesm force-pushed the fix/imap-delete-staged branch from ef50564 to 865cfbe Compare April 7, 2026 14:56
@wesm @claude
Fix IMAP account lookup by display name in delete-staged
3dfdec0 
IMAP source identifiers are URLs (imaps://user@host:port), not email
addresses. When --account is an email, resolve via
GetSourcesByIdentifierOrDisplayName and canonicalize to the stored
identifier before comparing against manifest filters.

Also moves DB open before account resolution so the lookup is available.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Apr 7, 2026

roborev: Combined Review (865cfbe)

Verdict: changes are not safe to merge yet due to 1 high-severity correctness issue and 1 medium-severity ambiguity issue.

High

  • cmd/msgvault/cmd/deletions.go:376 Deletion records are still keyed by source_message_id alone, which is incorrect for IMAP. IMAP UIDs are only unique within a mailbox/account, so deleting message 123 from one IMAP account can incorrectly mark unrelated messages with source_message_id = 123 in other accounts as deleted in the local DB.
    Fix: scope deletion marks by source_id plus source_message_id, and plumb the selected source/account identity into the executor.

Medium

  • cmd/msgvault/cmd/deletions.go:381 Source selection is ambiguous when multiple sources share the same identifier. The new lookup picks the first gmail or imap source it finds, so a staged batch for the same address could be executed against the wrong backend.
    Fix: persist source_id or at least source_type in the deletion manifest and require an exact match at execution time; if the manifest is ambiguous, fail clearly instead of choosing arbitrarily.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm @claude
Error on ambiguous --account match in delete-staged
2abb061 
When the --account value matches multiple syncable sources (e.g. both
a Gmail and IMAP source share the same email), return an explicit
ambiguity error listing all matches instead of silently picking the
first one.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Apr 7, 2026

roborev: Combined Review (2abb061)

Verdict: No medium-or-higher issues found in this PR.

The reviewed changes to cmd/msgvault/cmd/deletions.go and internal/store/sources.go appear clean based on the combined review outputs.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm wesm merged commit d629a1e into wesm:main Apr 7, 2026
4 checks passed
@wesm
Copy link
Copy Markdown
Owner

wesm commented Apr 7, 2026

thanks! I just did some roborev grinding, hopefully it still works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

delete-staged does not work with IMAP accounts

2 participants

@DominicHolmes @wesm